Pull request overview

Adds broker/admin support for batch deletion of topics and subscription groups via new remoting request codes and request bodies, plus client APIs and broker-side handling.

Changes:

• Introduce new request bodies and request codes for batch-delete operations.
• Implement broker processor logic to delete topic/group lists (including dedup + optional cleanup behaviors).
• Add persistence helpers for batch config deletion and tests covering the new admin paths.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Review by github-manager-bot

Summary

This PR addresses two related performance issues in AdminBrokerProcessor:

1. Removes synchronized from deleteTopic since topicConfigTable is already a ConcurrentHashMap
2. Adds batch deletion APIs (DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST) to reduce network round-trips

Findings

• [Good] AdminBrokerProcessor.java:769 — Removed synchronized from deleteTopic. The underlying topicConfigTable is a ConcurrentHashMap, so per-key remove is atomic. This eliminates unnecessary serialization of concurrent admin delete requests.
• [Good] AdminBrokerProcessor.java — Added deleteTopicList() and deleteSubscriptionGroupList() handlers following the existing *_LIST convention (consistent with UPDATE_AND_CREATE_TOPIC_LIST).
• [Good] DeleteTopicListRequestBody.java / DeleteSubscriptionGroupListRequestBody.java — Clean request body classes extending RemotingSerializable, following existing patterns.
• [Good] MQClientAPIImpl.java:2202-2220 / 2281-2301 — Client-side batch deletion methods properly implemented with timeout handling.
• [Good] RequestCode.java — Added DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST request codes.
• [Good] TopicConfigManager.java / SubscriptionGroupManager.java — Added deleteTopicList() and deleteSubscriptionGroupList() batch methods.
• [Good] AdminBrokerProcessorTest.java — Comprehensive test coverage including:
  • Empty list validation (returns INVALID_PARAMETER)
  • System topic rejection in batch
  • Happy path with multiple topics
  • Duplicate handling in batch
  • Batch persist verification

Analysis

The implementation is well-structured:

1. Backward compatibility: Existing single-item deletion APIs remain unchanged
2. Consistent patterns: Follows the *_LIST convention established by CreateTopicListRequestBody
3. Proper validation: Empty lists and system topics are rejected
4. Test coverage: Edge cases are well covered

Suggestions

1. Consider adding a maximum batch size limit to prevent excessive memory usage or long-running operations.
2. The batch persist test verifies deduplication — good attention to edge cases.

Verdict

✅ Approved — Well-designed performance optimization with proper validation and comprehensive test coverage.

Automated review by github-manager-bot

Review by github-manager-bot

Summary

Adds batch deletion APIs for topics (DELETE_TOPIC_IN_BROKER_LIST) and subscription groups (DELETE_SUBSCRIPTIONGROUP_LIST), with proper per-resource authorization, input validation, and atomic-all-or-nothing semantics.

Findings

• [Info] Authorization — Correctly emits one DELETE context per resource in the batch, preventing bypass when the body carries the target list instead of the header. This is a security-critical detail handled well.
• [Info] Error handling — Returns a structured error response with the list of failed items and the first exception. Good for debugging partial failures.
• [Info] Dedup — Uses LinkedHashSet to preserve insertion order while deduplicating. Reasonable choice.
• [Info] Refactoring — Extracts collectPopRetryTopics() from inline code in deleteTopic(). Good cleanup that avoids duplication.
• [Info] Cleanup order — Deletes client config and subscription first, then topic config last. Allows retry after partial failure.
• [Warning] AdminBrokerProcessor.deleteTopicInBrokerList() — The RemotingCommand response is created once at the top and reused. If deleteTopicInBroker internally sets response fields, ensure there's no state leakage between the success and error paths.
• [Warning] MQClientAPIImpl.deleteTopicInBrokerList() — The method creates the request body and sends it. Consider adding a timeout or batch size limit to prevent excessively large requests.

Suggestions

• Consider adding a max batch size constant (e.g., 100) to prevent abuse or accidental large requests.
• The DeleteTopicListRequestBody and DeleteSubscriptionGroupListRequestBody classes are clean. Consider adding @AllArgsConstructor for convenience if not already present.
• Cross-repo: If batch deletion is exposed in the dashboard or CLI, ensure those clients are updated to use the new APIs.

Overall: Well-structured feature with proper authorization and error handling. A few minor suggestions but nothing blocking.

Automated review by github-manager-bot

Review by github-manager-bot

Summary

Adds two new batch admin APIs — DELETE_TOPIC_IN_BROKER_LIST and DELETE_SUBSCRIPTIONGROUP_LIST — to AdminBrokerProcessor, following the existing *_LIST convention (e.g. UPDATE_AND_CREATE_TOPIC_LIST). Includes proper authorization context building for body-driven batch requests, new request body DTOs, and test coverage.

Findings

• [Info] Authorization handling for body-driven batch APIs is correctly implemented — the DefaultAuthorizationContextBuilder decodes the request body and emits one DELETE context per item, preventing the annotation-based path from producing an empty context list (which would bypass permission checks).
• [Info] Good test coverage in DefaultAuthorizationContextBuilderTest — verifies blank entries are filtered, correct resource types, and per-item DELETE action.
• [Warning] The batch deletion is not atomic — if one topic/group fails to delete (e.g. still in use), the remaining items in the list may still be deleted. Consider documenting this partial-failure behavior in the API response, or adding a failFast flag.
• [Info] LinkedHashSet for deduplication while preserving insertion order is a good choice for predictable behavior.
• [Info] New DeleteTopicListRequestBody and DeleteSubscriptionGroupListRequestBody follow the existing pattern of CreateTopicListRequestBody.

Suggestions

• Consider adding a response summary that reports per-item success/failure status (similar to how batch operations in other systems report partial results). This would help callers understand which items succeeded when partial failure occurs.
• Add a brief comment in AdminBrokerProcessor documenting the non-atomic semantics of batch deletion.

No blocking issues. Well-structured addition that follows existing conventions.

Automated review by github-manager-bot